Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix and move logic that updates adUnitsS2SCopy based sizeMapping results #2795

Merged
merged 1 commit into from
Jul 27, 2018

Conversation

jsnellbaker
Copy link
Collaborator

Type of change

  • Bugfix

Description of change

This PR:

  • adds a fix to the logic we were using to update the bidRequests.adUnitsS2SCopy object. This update ensured only valid bids were being included in the s2s request.
  • moved this logic so that the updated adUnitsS2SCopy object is properly reflected in the console log messages for easier debugging/troubleshooting.

Additional Context
Currently for s2s requests, we resync the adUnitsS2SCopy object based on the various bids that exist in the bidRequest objects. This resync is to remove any bids that were rejected by the sizeMapping/label filtering checks.

Keeping the adUnitsS2SCopy in sync is important, as this object (not the bidRequest) is used to build the POST request meant for PBS; thus this ensures only validated bids get sent to PBS.

Notes on Fix
The logic used for the sync operated on a combination of the bidder and adUnitCodes to match adUnit.bid with bidRequest.bids to know if they passed the sizeMapping checks. This logic was insufficient when the same adUnitCode was used across different adUnits that used different labels. It allowed bids for the wrong label to pass the screening and get added to the PBS request.

The new logic operates on the bid_id values, which are generated uniquely for each adUnit.bid object and then stored in the corresponding bidRequest.bid object.

By moving this logic to the makeBidRequest function, we can display the updated adUnitsS2SCopy object in the 'Bids Requested' log message (here https://github.com/prebid/Prebid.js/blob/master/src/auction.js#L188 ) instead of digging through debugger.

@mercuryyy
Copy link
Contributor

mercuryyy commented Jul 4, 2018

@jsnellbaker tested the pull found an issue.

The defined sizes for each label dont pass correctly into the adUnitsS2SCopy array.

So for example if we have

label1 = desktop - sizes = 728,90 300,250
label2 = mobile - sizes = 320,50, 300,250

If appnexus labelany: ['desktop','moble']

If fired from desktop prebid will pass all sizes to adUnitsS2SCopy insted of just "728,90 300,250"

  • When looking at Bids Requested array the Client side bids sizes are correct where in adUnitsS2SCopy they are not filtered .

@jsnellbaker
Copy link
Collaborator Author

Hi @mercuryyy Can you provide a markup of the adUnits array that replicated the issue you described above? I have been attempting a few configurations but I haven't replicated the issue yet.

@mercuryyy
Copy link
Contributor

@jsnellbaker Are you on the adops Slack channel or have an email i can DM a test page?

@jsnellbaker
Copy link
Collaborator Author

Hi @mercuryyy, you can send it to jsnellbaker@appnexus.com

Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@snapwich snapwich added the needs 2nd review Core module updates require two approvals from the core team label Jul 19, 2018
@jsnellbaker jsnellbaker requested a review from mkendall07 July 20, 2018 16:18
@jsnellbaker
Copy link
Collaborator Author

Update on issue mentioned here; I did investigate the test pages provided, but I wasn't able to see the issue described by @mercuryyy. There were other issues related to Prebid Server, but they were separate from this PR.

@mkendall07
Copy link
Member

LGTM. Thanks

@jsnellbaker jsnellbaker merged commit c2310ba into master Jul 27, 2018
@mkendall07 mkendall07 deleted the fix_sizeconfig_sync_for_s2sadunits branch August 17, 2018 15:12
florevallatmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Sep 6, 2018
StefanWallin pushed a commit to mittmedia/Prebid.js that referenced this pull request Sep 28, 2018
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
AlessandroDG pushed a commit to simplaex/Prebid.js that referenced this pull request Mar 26, 2019
AlessandroDG pushed a commit to simplaex/Prebid.js that referenced this pull request Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants